Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix primary.idx corruption after delete mutation #9048

Merged
merged 6 commits into from
Feb 9, 2020

Conversation

alesapin
Copy link
Member

@alesapin alesapin commented Feb 7, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix bug in ALTER DELETE mutations which leads to index corruption. This fixes #9019 and #8982. Additionally fix extremely rare race conditions in ReplicatedMergeTree ALTER queries.

@alesapin alesapin added pr-bugfix Pull request with bugfix, not backported by default no-docs-needed labels Feb 7, 2020
@alesapin
Copy link
Member Author

alesapin commented Feb 7, 2020

Unit tests — Segmentation fault -- already fixed on master, not related to changes.

@alexey-milovidov alexey-milovidov mentioned this pull request Feb 7, 2020
16 tasks
@akuzm
Copy link
Contributor

akuzm commented Feb 7, 2020

Did you find out why the order of rows changes? Theoretically, when reading sequentially in a single thread, it should be the same.

else
key_expr = metadata.order_by_ast;

bool empty = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this check for, something like order by tuple()? Can we just remove this check and pass the order by always, or does it break something? Btw I found that order by tuple() doesn't seem to work, because tuple requires at least one argument, but order by 1 does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we cannot order by tuple() and in this case we don't need order by at all.

@alesapin
Copy link
Member Author

alesapin commented Feb 8, 2020

Did you find out why the order of rows changes? Theoretically, when reading sequentially in a single thread, it should be the same.

Still not, but I'll try to find the next week. Anyway, when we use InterpreterSelectQuery and want to preserve order we have to add ORDER BY. We don't provide any guarantees even in a single-threaded case, and if "ordered" behavior was changed unintentionally ORDER BY is still required.

@tavplubix tavplubix merged commit 4543177 into master Feb 9, 2020
tavplubix added a commit that referenced this pull request Feb 9, 2020
Fix primary.idx corruption after delete mutation
@tavplubix tavplubix added the v20.1 label Feb 9, 2020
@tavplubix
Copy link
Member

Backport conflict in 19.17

tavplubix added a commit that referenced this pull request Feb 14, 2020
Fix primary.idx corruption after delete mutation
tavplubix added a commit that referenced this pull request Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken indicies and inconsistent queries results
3 participants